FIX: Avoid disabling project-wide actions on shutdown#2346
FIX: Avoid disabling project-wide actions on shutdown#2346AswinRajGopal wants to merge 4 commits intodevelopfrom
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
PR Code Suggestions ✨Explore these optional code suggestions:
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
|||||||||
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2346 +/- ##
===========================================
- Coverage 77.90% 77.89% -0.02%
===========================================
Files 476 476
Lines 97613 97638 +25
===========================================
+ Hits 76048 76056 +8
- Misses 21565 21582 +17 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
| UnregisterAction(ref m_ScrollWheelAction, OnScrollWheelPerformed); | ||
|
|
||
| if (m_InputActionAsset != null) | ||
| if (m_InputActionAsset != null && m_InputActionAsset != InputSystem.actions) |
There was a problem hiding this comment.
Do we actually have to disable the actions? I'm not a huge fan of disabling this for a particular asset, like project wide actions. I also think the logic we implemented in the past might not be the most correct.
I believe we should always make sure that the UI action map is enabled when registering the actions. Do you know what's the impact of not disabling this asset for a normal asset?
There was a problem hiding this comment.
@jfreire-unity Thanks for the review, I agree that the current change only addresses the reported bug. I’ll improve the fix so that we explicitly ensure the UI action map is enabled when registering actions, and on unregister we only clean up state owned by the provider instead of disabling shared asset state.
Description
case: https://issuetracker.unity3d.com/product/unity/issues/guid/UUM-134130
Issue:
UI Toolkit now cleans up its internal objects when no UI component exists in the scene. That means
InputSystemProvider.Shutdown()can be called during play.Shutdown()callsUnregisterActions(), which disablesm_InputActionAsset. When the provider is usingInputSystem.actions(project‑wide actions), this shuts off all input for the game. This is an external side effect and not expected when UI Toolkit goes away.Fix: Only disable the action asset on shutdown if it is not the project‑wide asset. This keeps the existing init/enable behavior unchanged, but avoids disabling InputSystem.actions when UI Toolkit is shutting down.
Testing status & QA
Added new test.
Verified manually using repro project.
Overall Product Risks
Comments to reviewers
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.